Add google analytics support#929
Conversation
crphang
force-pushed
the
integrate-google-analytics
branch
2 times, most recently
from
5757828 to
9765a61
Compare
crphang
changed the title
|
Ready for review. Would require additional help on functional testing portion. Not entirely sure what is the adopted/best practice. |
yamgent
left a comment
yamgent
left a comment
There was a problem hiding this comment.
Please rebase to the latest master, so that we can remove the unrelated changes such as test/functional/test_site/expected/diagrams/usecase.png.
Would require additional help on functional testing portion. Not entirely sure what is the adopted/best practice.
I think the current implementation of yours is the best we can do. If in the future, Google drastically changes the method of tracking users, we will just have to rely on user reports to update on our side.
crphang
force-pushed
the
integrate-google-analytics
branch
from
57c9ca5 to
b0e36b0
Compare
|
@yamgent made changes. Ready for review. Thanks for reviewing |
yamgent
left a comment
yamgent
left a comment
There was a problem hiding this comment.
Hmm interesting, the diagrams seems to change due to it being rendered differently (I presume it is because different OS renders the diagrams differently?)
Anyway, one minor nit, otherwise this seems good to go.
| @@ -0,0 +1,24 @@ | |||
| #### `Google Analytics`: Enhancing site with Google Analytics | |||
|
|
|||
| This plugin allows your web pages to be enhanced with [google analytics](https://analytics.google.com/analytics/web/#/) to track, analyse and improve your content.. | |||
There was a problem hiding this comment.
Remove the duplicate dots at the end.
What do we do about this? The diagrams shouldn't be part of this PR right? |
Yes, I am of the view that the changes should be discarded for now in this PR, since the original source code of the diagram did not change. If the source code changes, then the PR that made the changes should update the diagram (for that case, I think it is OK if the PR author is not using Windows to update them, since pictures need manual verification anyway...) |
Does that also mean every PR coming from a non-Windows contributor will be required to remove unrelated changes from the PR every time? 😨 |
A possible way to alleivate this issue for now is to get rid of PlantUML diagrams in the functional tests. Ideally, it would be great if someone could try and find ways to make the style consistent across all OS (I haven't research enough to say whether this is feasible with PlantUML though). |
crphang
force-pushed
the
integrate-google-analytics
branch
from
132e842 to
c964a88
Compare
|
@yamgent Removed diagram changes and typo. Ready for review. |
3 participants
What is the purpose of this pull request?
Resolves #355
Resolves #902
Remaining Tasks:
What is the rationale for this request?
To provide easy way for authors to add google analytics into their web built by markbind. This PR adds this support by adding additional Google Analytics plugin.
What changes did you make? (Give an overview)
Provide some example code that this change will affect:
Add to site.json accordingly
{ "plugins": ["googleAnalytics"], "pluginsContext" : { "googleAnalytics" : { "trackingID": "UA-143800593-1" } } }This will be further simplified with #930